-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancement: Units edition results #146
base: main
Are you sure you want to change the base?
Conversation
Merge branch 'main' into enhancement/units-edition-results # Conflicts: # R/export_cdisc.R # inst/shiny/tabs/nca.R # inst/shiny/tabs/outputs.R
Hello team @m-kolomanski @Gotfrid @js3110 ! I have a small technical problem with this When the package is installed through non-Windows OS, the installation through CRAN with There are many potential solutions to this, as the only affected things are the
Let me know what you think of them or if you could come up with a better solution and I will try to implement it! |
Hi @Gero1999, use whatever dependencies you need to use - there is no need to re-invent the wheel if ready solutions are available, and we are not the first to deal with system dependencies, so do not worry :) In terms of installing the package itself, if installing a dependency (in that case In terms of github actions / workflows, we can install the dependency on the machine that runs the checks, I will get on that. |
I agree with @m-kolomanski, adding the dependency is the best path. Writing your own unit handling will likely be error-prone. (I've done it a few times in the past, and it always has lots of bugs the first time around.) The units package has the side-benefit that all standard unit conversions should be handled for you. |
I think the feature itself is ready for review. The other conflicts will hopefully be solved once the workflow incorporates the dependency mentioned. Let me know if so far this convince you or you can think of improvements: Feat characteristics
|
…a_results.R Merge remote-tracking branch 'origin/main' into enhancement/units-edition-results # Conflicts: # DESCRIPTION # R/reshape_PKNCA_results.R # inst/shiny/data/DummyRO_ADNCA.csv # inst/shiny/tabs/nca.R # inst/shiny/ui.R
I ended up merging instead of rebasing because in practice seemed less messy to me (I needed way too many commtits when rebasing). Hope is fine! Regarding some aspects of the module simplicity @m-kolomanski, I tried to document better the changes that occur:
But please let me know any concern with these solutions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, it looks quite nice, but there are some minor issues to iron out.
The data flow is still a bit confusing, but I think we should fix that in the scope #144 , when modularizing the whole NCA tab.
inst/shiny/modules/units_table.R
Outdated
showModal(modalDialog( | ||
title = tagList( | ||
span("Units of NCA parameter results"), | ||
tags$button( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this button does not seem to do anything.
Co-authored-by: Mateusz Kołomański <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, to me, the code looks good, thank you!
Since this PR touches the nca.R
file directly, and we wish to rework it ASAP, there are some minor issues / suggestions that we should create a separate issue for and address within a new PR:
- Bug: the application crashes when a text value is provided as conversion factor;
- Suggestion: If conversion factor returns NA, might be worth displaying something else in the table, as opposed to an empty cell (like *Error or something, to explicitly inform user that there is something wrong).
- Suggestion: Disable the ability to select rows in unit table, as it does not provide any functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Gero1999
It looks really good! I love the layout especially.
However I have noticed that the nca results are missing a lot of parameters (such as CL clearance) , which is important as this parameter is the main reason we are trying to implement unit changes (for pre clinical)
I have had a check and the parameters are also missing in main, but not in our rsconnect version so it must have slipped through in the #109 merge.
if you can fix it in this issue then great, and if not then lets make a separate issue for this!
Looks to me as if this is coming from the interval creation, as if you look at mydata() it is set as FALSE for most of the parameters except for the ones included in the format_pkncadata_intervals function. We want all the parameters to be available, or the user should be able to choose. Currently neither are possible. |
I've created issue #168 as it is separate from this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue
Users would like to be able to choose the units in which the results parameters are displayed.
Closes #113
Description
Sometimes user might be interested in changing the standard format of units provided by the app. In one that this may happen frequently is for example clearance (cl.obs), where current units always make very small values (~0) that can confuse when the rounding is used in the summary statistics.
Definition of Done
How to test
How to test features not covered by unit tests.
Contributor checklist
System to change units in parameters works well before NCA
System to change units in parameters works well after NCA
Outputs post NCA still work fine and adapt well to new units (i.e, boxplots)
Code passes lintr checks
Code passes all unit tests
New logic covered by unit tests
New logic is documented
Notes to reviewer
Anything that the reviewer should know before tacking the pull request?